PrettyPrinter reports wrong line LineNumbersTests #883
Conversation
bkolb
requested review from
ahoppen,
allevato,
bnbarham and
shawnhyam
as code owners
bkolb
force-pushed
the
bug/882-wrongLineNumberWithComments
branch
2 times, most recently
from
0852ed5 to
9b47a88
Compare
bkolb
force-pushed
the
bug/882-wrongLineNumberWithComments
branch
from
9b47a88 to
028ad94
Compare
|
@bkolb Could you run swift-format on your changes? The Windows failures will be fixed by swiftlang/github-workflows#70. |
Head branch was pushed to by a user without write access
bkolb
force-pushed
the
bug/882-wrongLineNumberWithComments
branch
from
028ad94 to
1af4c17
Compare
Done |
|
No problem :-) Thx for your reviews. I don't understand the build failure on windows. Can you give me a hint? |
|
The Windows test failure is an infrastructure issue and will be fixed by swiftlang/github-workflows#70. |
|
@bkolb can we try changing https://github.com/swiftlang/swift-format/blob/main/.github/workflows/pull_request.yml#L10C5-L10C83 to |
The reason for the wrong line number were multiline comments. In to accomodate for this, we now check the string while writing for new lines and increment the line count accordingly. Issue: swiftlang#882
bkolb
force-pushed
the
bug/882-wrongLineNumberWithComments
branch
from
1af4c17 to
2cd032c
Compare
|
That |
|
Sorry for the delay. |
|
#894 contains instruction how to reproduce the measurement. |
|
Changing the code to: let lines = text.count { $0 == "\n" }
lineNumber += lines
guard lines > 1, let lastNewlineIndex = text.lastIndex(of: "\n") else {
// Handle case where no newline exists
column += text.count
return
}
let lastLine = text[text.index(after: lastNewlineIndex)...]
column = lastLine.countBrings the performance measurement to be much closer to the original. There may be a better way to get the last line though. (Updated for faster code) |
Worked to get the perfomance to be closer to where we were before the changes in swiftlang#883. This code should be only about 1.5% slower rather than 7% slower.
|
Thank you! I was just about to take a look. You have been faster :-) |
…yPrinter for swiftlang#894 Worked to get the perfomance to be closer to where we were before the changes in swiftlang#883. This code should be only about 1.5% slower rather than 7% slower. Using a lazy filter as `count(where:_)` isn't avaliable < Swift 6.0
…mance Optimized the PrettyPrinter for swiftlang#894 Worked to get the perfomance to be closer to where we were before the changes in swiftlang#883. This code should be only about 1.5% slower rather than 7% slower. Using a lazy filter as `count(where:_)` isn't avaliable < Swift 6.0. Evaluating the text in reverse to shave a few more instructions off.
…mance PrettyPrinterPerformance Optimized the PrettyPrinter for swiftlang#894 Worked to get the perfomance to be closer to where we were before the changes in swiftlang#883. This code should be only about 1.5% slower rather than 7% slower. Using a lazy filter as `count(where:_)` isn't avaliable < Swift 6.0. One forward loop and using the UTF8 view makes this faster than the original code pre-swiftlang#883.
This is to reproduce issue #882.
I am not sure if the fix is done in the right place. With my change every string is checked for new lines, which might be not necessary and comes at a slight cost. Maybe there is another, better place to fix this.